Implement block prefiltering for `Join`s of a lazy result + `IndexScan` #1647

RobinTF · 2024-11-27T16:58:23Z

With this PR, a join between a lazy intermediate result and an IndexScan prefilters the blocks of the IndexScan before reading them. Before this PR, this prefilter was only applied to joins between two IndexScans and between an IndexScan and a fully materialized intermediate result.

codecov · 2024-11-27T18:52:27Z

Codecov Report

Attention: Patch coverage is 97.79736% with 5 lines in your changes missing coverage. Please review.

Project coverage is 89.57%. Comparing base (5c0d28a) to head (9934c62).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/engine/IndexScan.cpp	97.87%	2 Missing and 1 partial ⚠️
src/engine/Join.cpp	97.36%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1647      +/-   ##
==========================================
+ Coverage   89.53%   89.57%   +0.04%     
==========================================
  Files         381      381              
  Lines       36649    36792     +143     
  Branches     4146     4170      +24     
==========================================
+ Hits        32813    32957     +144     
- Misses       2520     2522       +2     
+ Partials     1316     1313       -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

joka921

A first somewhat thorough pass on the code (not yet the tests).
I think I still found a bug, if you agree, fix it, and add a test that proves who of us was right.

src/engine/Join.h

src/engine/Join.cpp

src/engine/IndexScan.cpp

joka921

Some additional comments.

src/engine/IndexScan.cpp

joka921 · 2024-12-03T11:24:13Z

src/engine/IndexScan.cpp

+  rti.numRows_ = metadata.numElementsYielded_;
+  rti.totalTime_ = metadata.blockingTime_;
+  rti.addDetail("num-blocks-read", metadata.numBlocksRead_);
+  rti.addDetail("num-blocks-all", metadata.numBlocksAll_);
+  rti.addDetail("num-elements-read", metadata.numElementsRead_);
+
+  // Add more details, but only if the respective value is non-zero.
+  auto updateIfPositive = [&rti](const auto& value, const std::string& key) {
+    if (value > 0) {
+      rti.addDetail(key, value);
+    }
+  };
+  updateIfPositive(metadata.numBlocksSkippedBecauseOfGraph_,
+                   "num-blocks-skipped-graph");
+  updateIfPositive(metadata.numBlocksPostprocessed_,
+                   "num-blocks-postprocessed");
+  updateIfPositive(metadata.numBlocksWithUpdate_, "num-blocks-with-update");


Can you make this part also a member function of the LazyScanMetadata?
This is then better if we add additional members to it.

There is an abstraction that can be moved into LazyScanMetadata, but the part that calls updateRuntimeInformationWhenOptimizedOut has to be part of a class related to Operation and the compressed relation reader class currently does not include RuntimeInformation, which means a new header would need to be added that isn't really related to this class.

If you don't feel that you have the time right now, limit yourself to the comments that are important for the main functionality of this PR.

The real solution here is that neither the LazyScanMetadata nor the CompressedBlockMetadata should be part of the large CompressedRelation.h file.

joka921 · 2024-12-03T11:26:15Z

src/engine/IndexScan.cpp

+  Result::Generator generator_;
+  // The column index of the join column in the tables yielded by the generator.
+  ColumnIndex joinColumn_;
+  // Metadata and blocks of this index scan.
+  Permutation::MetadataAndBlocks metaBlocks_;
+  // The iterator of the generator that is currently being consumed.
+  std::optional<Result::Generator::iterator> iterator_ = std::nullopt;
+  // The index of the last matching block that was found using the join column.
+  std::optional<size_t> lastBlockIndex_ = std::nullopt;
+  // Values returned by the generator that have not been re-yielded yet.
+  std::deque<Result::IdTableVocabPair> prefetchedValues_{};
+  // Metadata of blocks that still need to be read.
+  std::vector<CompressedBlockMetadata> pendingBlocks_{};


Please reorder them, s.t. the :

Generator

iterator

joinColumn

prefetchedValues_

pendingBlocks

lastBlockIndex

the rest.

Then they are grouped correctly and easier to read.

The order is currently mainly a result of moving all non default initialized values to the top

Okay, in that case, for such an complex struct, write an explicit constructor which takes the arguments that you need and then you can order them semantically logic.

src/engine/IndexScan.cpp

joka921 · 2024-12-03T11:53:48Z

src/engine/Join.h

@@ -29,24 +29,14 @@ class Join : public Operation {
  vector<float> _multiplicities;

 public:
+  // `allowSwappingChildrenOnlyForTesting` should only ever be changed by tests.


How hard is it to get rid of the tests that require this?

Very hard. The other constructor with the "only for testing" tag was previously used, but I had to change it to make it work with the new function signatures to the point where it pretty much ended up like a second constructor that does almost the same.

joka921 · 2024-12-03T11:57:02Z

src/engine/Join.cpp

+    if (allowSwappingChildrenOnlyForTesting) {
+      std::swap(t1, t2);
+      std::swap(t1JoinCol, t2JoinCol);
+    }


Which tests does this break, and can they be fixed as part of this PR?

I don't have a super strong opinion here, you can leave it for now, s.t. this
becomes mergeable in a finite amount of time:)

All that use the "join lambda", see test/util/JoinHelpers.h

src/engine/Join.cpp

sparql-conformance · 2024-12-03T13:56:42Z

Conformance check passed ✅

No test result changes.

Details: https://qlever.cs.uni-freiburg.de/sparql-conformance-ui?cur=9934c620ad84ddef26920e43f55533606e3dd3ac&prev=5c0d28afc98e178c58745df056939ea6bf55d985

sonarcloud · 2024-12-03T15:21:14Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

joka921

Thank you very much!

This fixes a bug introduced in #1647, which led to a failure of ` AD_CORRECTNESS_CHECK(!prefetchedValues.empty() || innerState->doneFetching_);` in `IndexScan::createPrefilteredJoinSide`.

RobinTF added 8 commits November 26, 2024 18:27

Prefiltering first draft

36d1edf

Skip empty tables

e7b9746

Reduce code duplication

778da48

Unify join column mapping

9de0e71

Restructure code and add documentation

9cf4658

Fix tests

730a5bd

Add unit tests

e5751cf

Workaround compilation error on gcc

fea04c6

RobinTF added 4 commits November 27, 2024 22:54

Unify code and add more unit tests to increase coverage

c49988f

Fix sonarcloud issues

2f44623

Test for more edge cases

8d0d023

Cover newly introduced missing coverage lines

6505c03

joka921 requested changes Nov 29, 2024

View reviewed changes

RobinTF added 7 commits December 2, 2024 15:59

Merge remote-tracking branch 'ad-freiburg/master' into prefiltering

a1900e1

Add more documentation and address smaller PR comments

896d7cd

Fix bug, add optimization and add proper unit tests for it

515400d

Refactor code to have less duplication

d1d1c56

Simplify code

5b4ec8e

Fix comments, use aliases, remove redundant bool, add assertions

b7e37f0

Fix wrong join column

317127c

joka921 requested changes Dec 3, 2024

View reviewed changes

Address PR comments

9934c62

joka921 approved these changes Dec 3, 2024

View reviewed changes

joka921 changed the title ~~Implement prefiltering for lazy result + IndexScan~~ Implement block prefiltering for Joins of a lazy result + IndexScan Dec 3, 2024

joka921 merged commit 44e2ba8 into ad-freiburg:master Dec 3, 2024
23 checks passed

RobinTF deleted the prefiltering branch December 3, 2024 15:45

joka921 mentioned this pull request Dec 4, 2024

Fix the lazy block prefilter #1657

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement block prefiltering for `Join`s of a lazy result + `IndexScan` #1647

Implement block prefiltering for `Join`s of a lazy result + `IndexScan` #1647

RobinTF commented Nov 27, 2024 •

edited by joka921

Loading

codecov bot commented Nov 27, 2024 •

edited

Loading

joka921 left a comment

joka921 left a comment

joka921 Dec 3, 2024

RobinTF Dec 3, 2024

joka921 Dec 3, 2024

joka921 Dec 3, 2024

RobinTF Dec 3, 2024

joka921 Dec 3, 2024

joka921 Dec 3, 2024

joka921 Dec 3, 2024

RobinTF Dec 3, 2024

joka921 Dec 3, 2024

joka921 Dec 3, 2024

RobinTF Dec 3, 2024

sparql-conformance bot commented Dec 3, 2024

sonarcloud bot commented Dec 3, 2024

joka921 left a comment

Implement block prefiltering for Joins of a lazy result + IndexScan #1647

Implement block prefiltering for Joins of a lazy result + IndexScan #1647

Conversation

RobinTF commented Nov 27, 2024 • edited by joka921 Loading

codecov bot commented Nov 27, 2024 • edited Loading

Codecov Report

joka921 left a comment

Choose a reason for hiding this comment

joka921 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sparql-conformance bot commented Dec 3, 2024

Conformance check passed ✅

sonarcloud bot commented Dec 3, 2024

Quality Gate passed

joka921 left a comment

Choose a reason for hiding this comment

Implement block prefiltering for `Join`s of a lazy result + `IndexScan` #1647

Implement block prefiltering for `Join`s of a lazy result + `IndexScan` #1647

RobinTF commented Nov 27, 2024 •

edited by joka921

Loading

codecov bot commented Nov 27, 2024 •

edited

Loading